fix: group contiguous note edits into a single undo step#4524
Conversation
Route note edits through editThought instead of setDescendant so consecutive note changes merge in the undo stack, and mark toggleNote as non-undoable.
|
|
Issue A: Undo is splitted into 2 stacks for replacing a word on NotesStep to Reproduce
Current behaviorThe first Undo the replaced word becomes Fast.type.movSlow.Type.movExpected behaviorThe first Undo should revert to the original |
Issue B: Backspace / Delete is not working and caret is moved to the end of the thought text after Undo on the first thoughtStep to Reproduce
Current behaviorNothing happens. Issue.B.Backspace.Delete.is.not.working.after.Undo.MP4Expected behaviorBackspace/delete should work after Undo action. |
Issue C: Undo a new Notes creation is Undoing the last action prior the creating the new Notes but leaving the empty NotesStep to Reproduce
Current behavior
Issue.C.Undo.a.new.Notes.creation.is.Undoing.the.last.action.prior.the.creating.the.new.Notes.but.leaving.the.empty.Notes.MP4Expected behaviorNewly created empty note should be deleted |
Issue D: Backspace/Delete button on the keyboard deletes the thought space.Step to Reproduce
Current behaviorThought space is deleted. Issue.D.Backspace.Delete.button.on.the.keyboard.deletes.the.thought.space.MP4Expected behaviorBackspace/Delete button on the keyboard should serve to delete the character before the caret. |
There was a problem hiding this comment.
List of issues:
- Issue A: Undo is split into 2 stacks for replacing a word on Notes
- Issue B: Backspace / Delete is not working and caret is moved to the end of the thought text after Undo on the first thought
- Issue C: Undo a new Notes creation is Undoing the last action prior the creating the new Notes but leaving the empty Notes
- Issue D: Backspace/Delete button on the keyboard deletes the thought space.
@raineorshine
Question about Issue A, is this intended as the implementation of Replacements → separate undo steps
Refer to
Replacements → separate undo steps
#4479 (comment)
This raises a good question, though for the purpose of this issue we are just aiming to reproduce the undo/redo behavior that currently exists for normal thoughts. So for any edit to a note, check how many undo/redo steps are created if the same edit is applied to a normal thought. That should be enough to figure out the intended functionality, but to answer your specific question about Issue A, you have come up with a case that I don't think I had considered yet, and I'm not sure how this is handled in the code currently. I think the code currently categorizes edits based on the string length delta: ADD: Issue A is unique in that it is a replacement, but the number of characters are not the same. I think However, that's all out of scope for this issue, whose main purpose is to implement baseline undo/redo chunking to notes. This was understandably not so clear from my hasty bug report. |
|
@patbyte Is this ready for review or are you still working on it? Once ready, please use the GitHub "Request Review" feature to request a behavioral review from Bayu. Then I will review once behavior is confirmed. Thanks!
|
Issue E: Unable to partial select on the note textStep to Reproduce
Current behaviorSelection highlight is flashing and it is not selecting the text Issue.E.Unable.to.partial.select.on.the.note.text.movExpected behaviorUser should be able to select text with double click or click and drag |
|
Hi @BayuAri , What about the issues from A to D? |
I'll report that on my Review comment at the end of the testing. I log the issue per comment so we can reference this easily on the Review comment linking. |
BayuAri
left a comment
There was a problem hiding this comment.
@patbyte
Test result
List of issues:
Testing is blocked by Issue E
Fixed
Fixed
Partially fixed
This is considered as fixed because the caret is placed at the beginning of the notes, however, hitting Backspace when the caret is at the beginning of the Notes will delete the note and merge the notes text into thought text.
Step to Reproduce
- One - Two
- Create a note under Thought Two
- Add a text onto the Notes
- Place the caret at the beginning of the Notes text
- Hit Backspace on the keyboard
Current behavior
Notes text merges with the Thought Text.
Expected behavior
@raineorshine
Please help with the expected result.
This refers to the behavior of Thought but it is kind of weird when on Notes.
I think nothing should happen here.
This is not a regression issue but behavioral question because this exist onmain
New regression issue:
|
@BayuAri , Thanks for your testing. |

Route note edits through editThought instead of setDescendant so consecutive note changes merge in the undo stack, and mark toggleNote as non-undoable.
This PR fixes #4479.
Changes: